Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always close after assert_screen to make test stable. #24

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

tompng
Copy link
Member

@tompng tompng commented Sep 28, 2024

Fixes REAME and all test that closes before assertion.
Fixes #18

After #10, assert_screen can be called several times in a single test. We don't have to close before assert_screen anymore.
Closing before assert_screen has some problem. We need #15 to perform test that closes stdin before assertion, so the preferred form is to always close after all assertions.

start_terminal(...)
write(a)
assert_screen(b)
write(c)
assert_screen(d)
...
close

In #10, I forgot to change the order of assert_screen and close because test was passing, but it turned out to be flaky.

@YO4
Copy link

YO4 commented Sep 28, 2024

Would it be possible to move close within teardown?
If you always have to do close at the end it seems possible.
I am experimenting with the following code. (for fail/pass conditional execution)

module Yamatanooroti::WindowsTestCaseModule
  def self.included(cls)
    cls.instance_exec do
      teardown do
        CLOSE_OPERATIONS()
      end
    end
  end
end

This can deprecate close() and moves the actual processing into the teardown().
This approach works for test-unit, but requires def teardown for ruby's test, which may be invasive to the client code.

@tompng
Copy link
Member Author

tompng commented Sep 30, 2024

close in teardown

Thank you! I updated Yamatanooroti::TestMultiplatform to use both startup(start_terminal) and teardown(close). Tools that use Yamatanooroti can also adopt this approach in their own test.

This can deprecate close() and moves the actual processing into the teardown().

Doing close-in-teardown by default is not good at writing this kind of test.

def test_foobar
  start_terminal 5, 30, cmd1
  assert_screen foo
  close # manual close
  start_terminal 5, 30, cmd2
  assert_screen bar
  # auto close
end

We can provide auto close by another approach, similar to File.open, PTY.spawn. I think this is more ruby-ish and less invasive.

start_terminal do # Auto close API
  assertions_here
end # close here

start_terminal # Manual close API
assertions_here
close

# Comparison with File API
File.open(path, mode) do |f| # Auto close File API
  f.puts text
end

f = File.open(path, mode) # Manual close File API
f.puts text
f.close

Copy link
Member

@ima1zumi ima1zumi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ima1zumi ima1zumi merged commit 87b84c9 into ruby:master Oct 24, 2024
9 checks passed
@tompng tompng deleted the fix_flaky_close branch October 24, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to update API Reference
4 participants